This would set a barrier between the current execution layer and its parent, discarding the upper value and only propagating the new one downward through the tree. That's not necessarily a bad thing, but a user could very easily make the mistake of calling set again somewhere further into the request thinking it would allow a different path through the request to access the value, which is not the case. In my experience, it's better for the barrier to be applied in the constructor that way the object can't unexpectedly fork part-way through a request. Rather than constructing one AsyncLocal instance at the top-level, you'd construct a new one at the start of each request.

I know there's also the use-case of enabling isolated components of a system to share data without explicit communication, and I think the best way to achieve that is just using a factory that can be called to produce or retrieve AsyncLocal instances. In a way, you kind of already have this with the set and get, but I feel the naming is misleading and should be made clearer that set is forking the following async tree to use the new context object.

Rather than constructing one AsyncLocal instance at the top-level, you'd construct a new one at the start of each request.

With this API such usage should be considered a bad practice, because of performance reasons and tricky API manipulations. By the latter I mean that it's hard to share individual AsyncLocal instance with each request. It's much simpler to use a single instance to handle all requests. See this example: https://github.com/nodejs/node/pull/31016/files#diff-9a4649f1c3f167b0da2c95fc38953a1fR485

As for merging values or doing any other actions on value modification, that could (and, in my opinion, should) be done in user-land, i.e. in application code or libraries, because exact logic depends on particular use case.

In a way, you kind of already have this with the set and get, but I feel the naming is misleading and should be made clearer that set is forking the following async tree to use the new context object.

I think that .get()/.set(value) method names are quite intuitive for anyone who is familiar with the concept of asynchronous call chain. To help users, PR adds a couple of examples showing how values are propagated into the documentation.

For me, alternative method names, like .read()/.fork(value) or something like that are much less obvious.

I feel the opposite. The .get() and .set(value) names are not intuitive at all to someone trying to use the API to solve the need of having a unique context object per-request. It's capable of achieving that by using .set(value) at the start of the request to store an object and then only using .get() to retrieve and modify that object for the rest of the request, but that's very non-obvious from the naming or the docs. The only reason it makes sense to us is because we are already intimately familiar with the concept of async call trees and how this feature is implemented. An average user is not, and I'm almost 100% certain users will try to use this like ThreadLocal in Java and then be totally confused when different parts of their app get different data because they keep calling .set(value) to "update" the stored value; it looks a lot like setState(...) in React, and I suspect many people will treat it as such. I'm sure many users will also try to construct a new AsyncLocal within each request as that's a common practice with ThreadLocal in Java, again doing it the wrong way here.

In my opinion, a proper async context API should look something more like this:

// This corresponds roughly to the existing `AsyncLocal`
const contexts = new ContextManager()

function loadUser(auth) {
  const { onError, onUser } = contexts.current
  User.login(auth).then(onUser, onError)
}

function httpResponder(res) {
  contexts.current.onError = err => {
    res.status(500).end(err.message)
  }
  contexts.current.onUser = user => {
    res.status(200).end(user.token)
  }
}

app.post('/login', (req, res) => {
  // This corresponds to `.set(value)` and is closest
  // to what most people actually think of as a local.
  contexts.create()
  httpResponder(res)
  loadUser(req.headers.authorization)
})

It's functionally identical to what you have built, but more cleanly communicates the intent of the objects and methods.

The snippet looks like something that can be built on top of AsyncLocal API, but personally I find it less straightforward. AsyncLocal API, in its current state, allows building APIs on top of it, say, the one from the snippet or something similar to cls-hooked/continuation-local-storage API. So, I expect it to be mainly used by library authors, not all node users.

As for user confusion, I hope that it can be mitigated by good enough documentation.

I don't really like the manual cleanup required here. If someone decides to make a unique AsyncLocal object per-request it may be ambiguous where to close the object and they might even not close it at all, leading to a memory leak. It'd be better to use a WeakSet so you don't have to worry about explicit cleanup.

WeakSet won't do here, as it doesn't allow iteration over its items. A Set with WeakRefs + FinalizationGroup API could potentially help to achieve automatic AsyncLocal clean up, but FG API is experimental and has to be enabled via a flag.

On the other hand, other CLS API implementations do not provide a way to free underlying resources and disable hooks. That's because most applications use a single instance of CLS without getting rid of it. So, AsyncLocal.remove() method is an advanced API, useful in specific scenarios.

WDYT?

There's ways to eliminate the global Set entirely by making the WeakMap global and propagating on that directly.

A global WeakMap used to store values won't allow to isolate different AsyncLocals, which kills the whole idea.

If you mean a WeakMap that stores <AsyncLocal, WeakMap> pairs, then it's not possible to iterate over WeakMap entries.

I mean something like WeakMap<AsyncResource, Set<AsyncLocal>>. The hooks can loop over the set of AsyncLocal instances at the current level and propagate them downwards. It means an array at each level, rather than just the top-level, but it also cleans up automatically.

That's an interesting idea. Another level of indirection with a WeakMap<AsyncResource, Set<AsyncLocal>> will help to clean up both values and AsyncLocals list in the hook. As downsides I can see the following:

• The hook will have to be kept always enabled, once the first AsyncLocal instance was created. Currently .remove() disables the hook if there are no active AsyncLocals.
• This change will probably have a certain performance impact. But that has to be measured.

In general, it sounds like a nice idea for an experiment. I'm going to do that experiment, if this PR gets some chances to be reviewed and merged (I understand that chances are close to zero, especially considering #30959 (comment)).